Implement SIMD funnel shifts in const-eval/Miri #147534
Conversation
rustbot
added
S-waiting-on-review
| } | ||
| let inv_shift_bits = (elem_size_bits - shift_bits) as u32; | ||
|
|
||
| let result_bits = if is_left { |
There was a problem hiding this comment.
This needs a comment explaining why this logic, executed on u128, correctly implements the semantics for all integer types.
There was a problem hiding this comment.
i added a comment, could you check if its enough
|
We can't test in const-eval yet right? This doesn't make the intrinsics |
|
Oh right, this has the impl in rustc but doesn't expose it yet as that is blocked or more extensive testing... yeah a Miri test also works. |
|
☔ The latest upstream changes (presumably #148446) made this pull request unmergeable. Please resolve the merge conflicts. |
sayantn
force-pushed
the
simd-funnel-shifts
branch
from
3947340 to
b3afd22
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
|
Those tests check that we find the UB; please also add tests ensuring we return the correct result (including the corner case |
There was a problem hiding this comment.
I edited the comment a little. You didn't talk about the implementation doing << and >> and | at all! You were supposed to explain why those are the right operations to use. Imagine explaining this to someone who has never seen a funnel shift before in their life (i.e., someone like me).
But even with all that, I still don't get it. You basically say "because they live in the lower bits, the implementation is easy". That explains nothing, it just asserts without justification that it is easy. What is the argument for why the bits end up in the right place?
|
☔ The latest upstream changes (presumably #148507) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Since there's more work happening in this direction, what are your thoughts on adding a to/from array conversion for all simd types via a trait and adding that trait bound to the intrinsic? Then the intrinsic body could be written in library code and work for all backends that do not implement the simd intrinsics (#93145) |
|
Yeah if we could have fallback impls for the |
sayantn
force-pushed
the
simd-funnel-shifts
branch
from
b3afd22 to
87bd458
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
RalfJung
added
S-waiting-on-author
sayantn
force-pushed
the
simd-funnel-shifts
branch
from
87bd458 to
b767368
Compare
sayantn
force-pushed
the
simd-funnel-shifts
branch
from
b767368 to
50a6df4
Compare
|
I have added a lot more explanation, could you check if that is enough? @rustbot ready |
rustbot
added
S-waiting-on-review
5 participants
Split off from #147520 with just this change for easier review
r? @RalfJung